Components: Refactor SlotFill #19242

diegohaz · 2019-12-19T14:54:01Z

Description

This PR is part of #18619.

The problem

There are several problems with the current slot-fill implementation. For example, the context value never changes because it's a class property whose value is never updated during the SlotFillProvider lifecycle:

gutenberg/packages/components/src/slot-fill/context.js

Lines 38 to 47 in 56cd1df

    
           this.contextValue = { 
        
           	registerSlot: this.registerSlot, 
        
           	unregisterSlot: this.unregisterSlot, 
        
           	registerFill: this.registerFill, 
        
           	unregisterFill: this.unregisterFill, 
        
           	getSlot: this.getSlot, 
        
           	getFills: this.getFills, 
        
           	hasFills: this.hasFills, 
        
           	subscribe: this.subscribe, 
        
           };

Because of that, both the Slot and the Fill components will never re-render when some action is done in the context (for example, when calling registerSlot or registerFill). Workarounds like forceUpdate() have been introduced to the code to overcome this:

gutenberg/packages/components/src/slot-fill/context.js

Lines 55 to 65 in 56cd1df

    
           // Sometimes the fills are registered after the initial render of slot 
        
           // But before the registerSlot call, we need to rerender the slot 
        
           this.forceUpdateSlot( name ); 
        
           // If a new instance of a slot is being mounted while another with the 
        
           // same name exists, force its update _after_ the new slot has been 
        
           // assigned into the instance, such that its own rendering of children 
        
           // will be empty (the new Slot will subsume all fills for this name). 
        
           if ( previousSlot ) { 
        
           	previousSlot.forceUpdate(); 
        
           }

Also, the Fill component won't re-render when new fillProps are passed to the Slot component (probably, a new forceUpdate call should be placed here to work around this issue):

gutenberg/packages/components/src/slot-fill/fill.js

Lines 56 to 59 in 56cd1df

    
           // If a function is passed as a child, provide it with the fillProps. 
        
           if ( isFunction( children ) ) { 
        
           	children = children( slot.props.fillProps ); 
        
           }

This solution

This is a rewrite of the slot-fill module using React Hooks and exposing only the Portal API (activated by using the bubblesVirtually prop today). This greatly simplifies the code without workarounds.

The component should behave exactly the same as when using the bubblesVirtually prop today, except that it will properly re-render when fillProps are updated.

The idea is to gradually migrate the codebase to use this while this is experimental and then replace the current slot-fill with this one if the experiment succeeds. This would also validate whether the current usage of slots without bubblesVirtually could use it or not.

How has this been tested?

This was tested using Storybook and unit tests.

Types of changes

New feature

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR. .

packages/components/src/slot-fill2/context.js

packages/components/src/slot-fill2/fill.js

storybook/test/__snapshots__/index.js.snap

diegohaz · 2020-02-25T03:07:31Z

Follow-up PR with improved tests: #20428

gziolo

I can confirm it fixes the issue I raised yesterday. Awesome work on fixing it. I believe it closes more existing issues. I will try to identify them :)

Let's ship it 🚢

aduth · 2020-03-02T16:29:45Z

This looks really cool! I'd like to circle back and dive into the specific changes, but at least on a cursory glance, the implementation looks solid.

Would you happen to have a sense whether this will help unblock #14845 / #17355 ? I noticed some earlier comments at #17355 (comment) , but those were prior to some additional changes that came later in this pull request.

draganescu · 2020-03-27T14:12:05Z

Advised by @aduth I tried reverting this PR and now this #18560 new editor crash is fixed. So this PR apparently did something that crashes the editor with the console message:

TypeError: "slot is undefined"

when doing this:

aduth · 2020-03-27T14:20:45Z

packages/components/src/slot-fill/bubbles-virtually/slot-fill-provider.js

+			const { [ name ]: slot, ...nextSlots } = prevSlots;
+			// Make sure we're not unregistering a slot registered by another element
+			// See https://github.com/WordPress/gutenberg/pull/19242#issuecomment-590295412
+			if ( slot.ref === ref ) {


Should it be safe to assume slot is defined here?

In debugging the issue mentioned by @draganescu at #18560 (comment) , the exact error occurs at this line of code:

It's not yet clear to me if the bug is in how the slot came to be undefined in the first place, or if we should be guarded in how we expect slot to be available at this line of code.

…lesVirtually from the toolbar group slot Removing bubblesVirtually from the toolbar group slot prevents any dynamically added toolbar buttons from firing their click event. This can be seen when cropping an image, you press Crop, then the Apply and Cancel buttons get added to the toolbar. Those button events don’t fire properly and is mentioned as an issue in the bubblesVirtually slot fill PR: #19242

diegohaz added 2 commits December 19, 2019 11:51

SlotFill initial implementation

6061ecd

Add manifest-devhub.json

7e38b5a

diegohaz mentioned this pull request Dec 19, 2019

Components: Improve Toolbar #18619

Closed

7 tasks

diegohaz added 6 commits December 19, 2019 19:39

Accept as prop on Slot

d032bee

Update stories

13b60ef

Update README.md

0c77a8a

Update code

bd84603

Add slot-fill2 entries to components index file

23d3abc

Add unit tests

229d432

diegohaz commented Dec 21, 2019

View reviewed changes

packages/components/src/slot-fill2/context.js Outdated Show resolved Hide resolved

diegohaz commented Dec 21, 2019

View reviewed changes

packages/components/src/slot-fill2/fill.js Outdated Show resolved Hide resolved

diegohaz commented Dec 21, 2019

View reviewed changes

storybook/test/__snapshots__/index.js.snap Outdated Show resolved Hide resolved

diegohaz added [Package] Components /packages/components [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible Storybook Storybook and its stories for components labels Dec 21, 2019

diegohaz marked this pull request as ready for review December 21, 2019 06:01

diegohaz requested review from ajitbohra, chrisvanpatten, jorgefilipecosta, talldan and youknowriad as code owners December 21, 2019 06:01

diegohaz self-assigned this Dec 21, 2019

diegohaz added [Type] Enhancement A suggestion for improvement. and removed Storybook Storybook and its stories for components [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible labels Dec 21, 2019

diegohaz requested a review from gziolo January 9, 2020 22:28

gziolo requested review from aduth and ellatrix January 13, 2020 08:38

ItsJonQ mentioned this pull request Jan 14, 2020

Dropdown in Modal Doesn't Work #19453

Closed

Merge branch 'master' into update/slot-fill

7aff149

diegohaz mentioned this pull request Feb 25, 2020

Add react-testing-library #20428

Merged

6 tasks

gziolo approved these changes Feb 25, 2020

View reviewed changes

diegohaz merged commit e8f4f37 into master Feb 26, 2020

github-actions bot added this to the Gutenberg 7.7 milestone Feb 26, 2020

diegohaz mentioned this pull request Feb 26, 2020

Fix intermittent RichText e2e test failure #20457

Merged

brentswisher mentioned this pull request Feb 28, 2020

Storybook: Next steps for better experience when developing UI components #17973

Closed

8 tasks

diegohaz deleted the update/slot-fill branch March 2, 2020 13:02

jorgefilipecosta added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Mar 16, 2020

jorgefilipecosta mentioned this pull request Mar 16, 2020

Using customClassName: false produces a weird bug #20874

Closed

jorgefilipecosta pushed a commit that referenced this pull request Mar 16, 2020

Components: Refactor SlotFill (#19242)

9b7eb2d

jorgefilipecosta mentioned this pull request Mar 16, 2020

Cherry Pick PR's for WordPress 5.4 RC 3 #20937

Merged

jorgefilipecosta pushed a commit that referenced this pull request Mar 16, 2020

Components: Refactor SlotFill (#19242)

c8c6fa1

jorgefilipecosta removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Mar 18, 2020

aduth reviewed Mar 27, 2020

View reviewed changes

aduth mentioned this pull request Mar 27, 2020

Components: SlotFill: Guard property access to possibly-undefined slot #21205

Merged

diegohaz mentioned this pull request May 6, 2020

Deprecate the original slot implementation and refactor usage #22027

Closed

StevenDufresne mentioned this pull request Jun 9, 2020

Slot/Fill - updateSlot is missing from Slot Fill Context. #23014

Closed

diegohaz mentioned this pull request Feb 24, 2021

Components: Try to make the order of fills stable in regular slots #29287

Merged

7 tasks

jeryj mentioned this pull request Apr 10, 2023

POC Slot/Fill Ordering #49401

Draft

jsnajdr mentioned this pull request Apr 26, 2023

SlotFill: several code cleanups #50098

Merged

jsnajdr mentioned this pull request May 10, 2023

BlockControls, InspectorControls: remove useSlot, unify behavior on bad group #50198

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Components: Refactor SlotFill #19242

Components: Refactor SlotFill #19242

diegohaz commented Dec 19, 2019 •

edited

Loading

diegohaz commented Feb 25, 2020

gziolo left a comment

aduth commented Mar 2, 2020

draganescu commented Mar 27, 2020 •

edited

Loading

aduth Mar 27, 2020

	this.contextValue = {
	registerSlot: this.registerSlot,
	unregisterSlot: this.unregisterSlot,
	registerFill: this.registerFill,
	unregisterFill: this.unregisterFill,
	getSlot: this.getSlot,
	getFills: this.getFills,
	hasFills: this.hasFills,
	subscribe: this.subscribe,
	};

	// Sometimes the fills are registered after the initial render of slot
	// But before the registerSlot call, we need to rerender the slot
	this.forceUpdateSlot( name );

	// If a new instance of a slot is being mounted while another with the
	// same name exists, force its update _after_ the new slot has been
	// assigned into the instance, such that its own rendering of children
	// will be empty (the new Slot will subsume all fills for this name).
	if ( previousSlot ) {
	previousSlot.forceUpdate();
	}

	// If a function is passed as a child, provide it with the fillProps.
	if ( isFunction( children ) ) {
	children = children( slot.props.fillProps );
	}

Components: Refactor SlotFill #19242

Components: Refactor SlotFill #19242

Conversation

diegohaz commented Dec 19, 2019 • edited Loading

Description

The problem

This solution

How has this been tested?

Types of changes

Checklist:

diegohaz commented Feb 25, 2020

gziolo left a comment

Choose a reason for hiding this comment

aduth commented Mar 2, 2020

draganescu commented Mar 27, 2020 • edited Loading

aduth Mar 27, 2020

Choose a reason for hiding this comment

diegohaz commented Dec 19, 2019 •

edited

Loading

draganescu commented Mar 27, 2020 •

edited

Loading